Skip to content

Wire: fix setClock #46

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 10, 2023
Merged

Wire: fix setClock #46

merged 2 commits into from
Aug 10, 2023

Conversation

facchinm
Copy link
Member

clock source is fixed for RA4M1, so the rates will be wrong for C33.
This will be fixed in the next commit

@mjs513
Copy link
Contributor

mjs513 commented Jul 14, 2023

@facchinm
Have a question on the settings that you are using for 400khz. If I attach a Logic analyzer to SDA/SCL while running a sketch pulling data from a MPU-9250 I am seeing:
Minima400currentSettings
which I believe is showing a frequency of about 257khz not close to the 400khz.

However if I change your settings to:

          m_i2c_extend.clock_settings.brl_value = 12;
          m_i2c_extend.clock_settings.brh_value = 11;
          m_i2c_extend.clock_settings.cks_value = 0;

I get closer to 400khz (357khz):
Minima400mysettings

EDIT: see post 8 and 9 of this thread for more info: https://forum.arduino.cc/t/i2c-setclock-does-not-appear-to-be-standard-way-to-setclock/1147122/6

@mjs513
Copy link
Contributor

mjs513 commented Jul 26, 2023

@facchinm
These settings work better - gives 400khz with 60% duty cycle

        case I2C_MASTER_RATE_FAST:
          m_i2c_extend.clock_settings.brl_value = 16;
          m_i2c_extend.clock_settings.brh_value = 15;
          m_i2c_extend.clock_settings.cks_value = 0;

@greiman
Copy link

greiman commented Jul 28, 2023

setClock should not only allow the three values, 100kHz, 400kHz, 1Mhz. See this issue.

clock source is fixed for RA4M1, so the rates will be wrong for C33.
This will be fixed in the next commit
@facchinm
Copy link
Member Author

facchinm commented Aug 4, 2023

@mjs513 @greiman I pushed a new version of the patch which should take care of the clocks for both UNO and C33.
In this patch I explicitly disabled FAST_MODE_PLUS for platforms not supporting it (eg. RA4M1) so we are in the specifications of the chip.
Let me know if it works for you, then we can merge it safely.

@mjs513
Copy link
Contributor

mjs513 commented Aug 4, 2023

@facchinm
Got some strange things happening when testing the changes at 400khz.
Using a 5v liquidCrystal_i2c device on Wire with your current settings of 16/15 I am seeing 500khz using a LA:
Screenshot 2023-08-04 144853
If I change the settings for 400khz to 22/21 I am seeing 400khz, again for 5v device on Wire:
Screenshot 2023-08-04 145134

But if I use those settings 22/21 for a MPU-9250 running at 3.3v on Wire I am seeing something like 257khz
Screenshot 2023-08-04 145849

Using your settings, again on wire for a MPU-9250 I am getting 294khz:
Screenshot 2023-08-04 150222

Confused!!!!!!

And if I move the 9250 over to Wire1 using your settings of 16/15 I am seeing that 526khz again
Screenshot 2023-08-04 150740

and if I use the 22/21 settings I am seeing 416khz at 52% duty cycle.

So now I am confused. Please note I do have the Micros propose PR incorporated as well as a couple of others.

Thanks for working this - getting better and better.

EDIT: all I did was copy and paste the updated Wire.cpp file.

@mjs513
Copy link
Contributor

mjs513 commented Aug 4, 2023

@facchinm
out of curiosity I pulled out a T4.1 (had it handy) and check 100khz and 400khz. At 100khz seeing 68khz with 50% while at 400khz I am seeing 357khz clock with 50% duty cycle for my MPU-9250.

Never one to leave enough alone:
Adafruit Metro M0 express: 97khz with 50% duty at 100Khz; 357khz with 50% duty at 400khz
Uno R3 Clone: 370khz with 52% duty at 400khz; 100khz with 49% duty at 100khz setclock.

@greiman
Copy link

greiman commented Aug 5, 2023

Wire1 has level shifters and works correctly for 3V3 devices. Wire is marginal for 3V3 devices. For 3V3 devices on Wire, the speed depends on pull-up resistance, pull-up voltage, and VCC for the RA4M1. See this.

@per1234 per1234 added type: imperfection Perceived defect in any part of project topic: code Related to content of the project itself labels Aug 8, 2023
@per1234 per1234 linked an issue Aug 8, 2023 that may be closed by this pull request
@mjs513
Copy link
Contributor

mjs513 commented Aug 8, 2023

@facchinm - just wanted to know if there was anything else you wanted me to try with this?

pennam pushed a commit to pennam/ArduinoCore-renesas that referenced this pull request Aug 9, 2023
Implement CAN FD support for Portenta H33 arduino#2
@mjs513
Copy link
Contributor

mjs513 commented Aug 9, 2023

@facchinm - maybe its a good idea to go ahead and incorporate the current fix (this pr) into the core. Seems to be resolve some issues people are having: https://forum.arduino.cc/t/i2c-setclock-does-not-appear-to-be-standard-way-to-setclock/1147122/19

@aentinger
Copy link
Contributor

As @facchinm is currently out-of-office I'll move ahead and merge this one.

@aentinger aentinger merged commit 1c9b086 into arduino:main Aug 10, 2023
cristidragomir97 pushed a commit to cristidragomir97/ArduinoCore-renesas that referenced this pull request May 20, 2024
Implement CAN FD support for Portenta H33 arduino#2

Former-commit-id: 6f20171
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself type: imperfection Perceived defect in any part of project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bug in Wire timeout loop causes long delays in I2C read/write.
5 participants